Skip to content

Create TypeScript declarations from JSDoc #45

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Jun 10, 2020
Merged

Create TypeScript declarations from JSDoc #45

merged 7 commits into from
Jun 10, 2020

Conversation

tinovyatkin
Copy link
Member

@tinovyatkin tinovyatkin commented Jun 8, 2020 •
edited
Loading

This PR drops manually created and maintained TypeScript typing in favor of automatically generated from JSDoc.
Run npm run prepublishOnly to inspect new one.
Also drops reference to dom.lib as discussed in node-fetch.

@tinovyatkin tinovyatkin changed the title typings-from-jsdoc Create TypeScript declarations from JSDoc Jun 8, 2020
@codecov

This comment has been minimized.

Sign in to view
@tinovyatkin tinovyatkin requested a review from jimmywarting June 8, 2020 02:12
LinusU
LinusU reviewed Jun 8, 2020
View reviewed changes
package.json Outdated
@@ -11,7 +11,8 @@
"lint": "xo",
"test": "xo && ava",
"report": "nyc ava",
"coverage": "nyc --reporter json --reporter text ava && codecov -f coverage/coverage-final.json"
"coverage": "nyc --reporter json --reporter text ava && codecov -f coverage/coverage-final.json",
"prepublishOnly": "npx typescript --declaration --emitDeclarationOnly --allowJs index.js"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will use the globally installed version if there is one, or download the latest version. I think it would be better if we could pin a version by adding typescript to our devDependencies, and then referring to that one here:

Suggested change
"prepublishOnly": "npx typescript --declaration --emitDeclarationOnly --allowJs index.js"
"prepublishOnly": "tsc --declaration --emitDeclarationOnly --allowJs index.js"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can pin TypeScript version at npx: npx typescript@latest ... if a lower version on a published system is the concern? We need it only before publishing, so, not sure if it better to include and maintain TypeScript as a dependency...

@tinovyatkin tinovyatkin requested a review from LinusU June 8, 2020 15:47
@tinovyatkin
Copy link
Member Author

@LinusU @jimmywarting merging?

@LinusU
Copy link
Member

LinusU commented Jun 9, 2020

I personally feel that this should be a dev-dependency instead of installed on demand using npx. Since this happens on pre-publish we would be shipping types generated by the latest version of TypeScript, without even reviewing how they look. A newer version of TypeScript could change something, and we wouldn't be able to see that until after the package is published 🤔

Is there any downside to adding typescript to our devDependencies?

@tinovyatkin
Copy link
Member Author

Is there any downside to adding typescript to our devDependencies?

Well, they almost the same as benefits:

  1. npm install in CI/on a developer machine will run slower by installing unneeded dependency
  2. A newer version of TypeScript could produce better output, but we will be unaware as we pinned an old one.

I don't understand why you will not be able to review output if it will be produced by the same command npm run prepublishOnly?

@tinovyatkin tinovyatkin mentioned this pull request Jun 10, 2020
Closed
35 tasks
@xxczaki
Copy link
Member

xxczaki commented Jun 10, 2020 •
edited
Loading

A newer version of TypeScript could produce better output, but we will be unaware as we pinned an old one.

@tinovyatkin Maybe we can add "typescript": "*" to devDependencies so that it will always use the latest version?

@xxczaki xxczaki added the enhancement New feature or request label Jun 10, 2020
@tinovyatkin
Copy link
Member Author

@xxczaki I've added typescript into devDependencies (with pinned version, otherwise it makes no sense at all), just to move this PR forward.

I understand that npx is relatively new command and seems strange sometimes, but it's a standard way of lazy-loading dependencies that are one-off commands (https://medium.com/@maybekatz/introducing-npx-an-npm-package-runner-55f7d4bd282b) and still believe that original proposal was better. Anyway, I'm up for whatever team decides and it's not a big deal.

@xxczaki xxczaki merged commit a1fef8c into master Jun 10, 2020
@xxczaki xxczaki deleted the typings-from-jsdoc branch June 10, 2020 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimmywarting jimmywarting jimmywarting approved these changes

@xxczaki xxczaki xxczaki approved these changes

@LinusU LinusU Awaiting requested review from LinusU

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tinovyatkin @LinusU @xxczaki @jimmywarting